Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add servicemonitor for prometheus #559

Merged
merged 4 commits into from
Oct 12, 2023

Conversation

dani-CO-CN
Copy link
Contributor

@dani-CO-CN dani-CO-CN commented Oct 7, 2023

Issue number:
Closes #558

Description of changes:
Add a named port for the controller-service and add an optional prometheus servicemonitor

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

Copy link
Contributor

@cbgbt cbgbt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution!

Do you think it makes sense to place the prometheus.io/scrape annotations behind a configuration flag, defaulted to enabled? You don't have to implement this, I've just worked with more charts since we wrote this one and have noticed that in a few that I've used.

deploy/charts/bottlerocket-update-operator/values.yaml Outdated Show resolved Hide resolved
deploy/charts/bottlerocket-update-operator/values.yaml Outdated Show resolved Hide resolved
@dani-CO-CN
Copy link
Contributor Author

Thanks for this contribution!

Do you think it makes sense to place the prometheus.io/scrape annotations behind a configuration flag, defaulted to enabled? You don't have to implement this, I've just worked with more charts since we wrote this one and have noticed that in a few that I've used.

We could also move the annotations to the values.yaml and then to a toYaml on the Service, this would allow overwriting.

prometheus:
  controller:
    service:
      annotations: 
        prometheus.io/port: "8080"
        prometheus.io/scrape: "true"
apiVersion: v1
kind: Service
metadata:
  annotations:
    {{- toYaml .Values.prometheus.controller.service.annotations | nindent 4 }}
  labels:
...

Copy link
Contributor

@cbgbt cbgbt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! 🚀

We could also move the annotations to the values.yaml and then to a toYaml on the Service, this would allow overwriting.

Good idea, I do like that better. I cut #566 to track that.

@gthao313
Copy link
Member

@dani-CO-CN LGTM, would you mind solving the conflicts? Otherwise, I can help on doing it. thanks!

@dani-CO-CN
Copy link
Contributor Author

@gthao313 conflicts are resolved! 🚀

@gthao313 gthao313 merged commit f428336 into bottlerocket-os:develop Oct 12, 2023
2 checks passed
@cbgbt cbgbt mentioned this pull request Jun 25, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Prometheus ServiceMonitor to the operator helm chart
4 participants